-
Notifications
You must be signed in to change notification settings - Fork 11k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[5.4] Force reconnect #15867
[5.4] Force reconnect #15867
Conversation
I'm not sure I understand the test you wrote, don't think it's testing anything! Also how would you use this new parameter to force reconnecting? |
Yes the test is dummy: it checks that exception is not raised as opposed to |
I have my example of how the new parameter is used here: #15865 (comment) |
This feels really strange and like we're avoiding some other problem. Why would we want to have a "force" parameter on a |
Currently when we tell disconnect, it means "disconnect if and only if not
in the middle of a transaction". Maybe this is wrong in the first place. If
it is for the sake of English language, I am not a native speaker. My
concern was to keep BC and not to introduce another function to do
"disconnect even if within transaction". Don't know if it is the best way
but I feel something needs change, doesn't it?
|
Another option, that may be more appropriate is let disconnect() do
"disconnect no matter what" and then those parts of code that need to
disconnect iff not in a transaction do "if not in transaction then
disconnect else throw exeption". That may be a new function if many
duplicates?
|
I would be curious to know why we are checking if we are already in a transaction and throwing that exception in the first place. |
Not sure, but sounds like it relates to the idea of automatically retrying
queries if they failed because of lost connection. Then a safegaurd is
added for not retrying if in the middle of a transaction, because when
connection is lost, the transaction is also lost, hence retrying is partial
and terrible. But the implementation of the check seems to be in a wrong
place.
|
To fix #15865